Hi @nickw444, thanks for the PR. I've had a look, but I'm still pondering some design/usability concerns.

1. Foremost, I think, is that I think (I haven't stewed on this sufficiently to be sure) that I'd prefer if the entirety of the completion script were generated by Kingpin (eg. via a --bash-completion-script flag), rather than having a separately installed script. This is a usability concern. This would also allow the completion implementation to change.
2. I'm not 100% convinced that calling into the app to do completion is the best approach (vs. generating the full completion script up front).
3. It would be ideal if flags with values had = appended (eg. --expires= vs. --expires) and didn't add a space. I had a quick stab at this, but couldn't figure out how to tell bash not to add a space after completing.

I really like the hint approach, this could also be useful for eg. generating more useful help.

Anyway, I'll have a bit more of a think about it, but at the moment I'm thinking that if 1. above is addressed, I'll probably just merge it because it's a great start, then if the underlying implementation changes it will be easy to do.

Also, I think the hint methods could use some documentation.

Hey @alecthomas thanks for the comments. I understand your concerns in all of your above points.

1.

I've moved the "support" scripts into the templates.go file and implemented 2 new flags to generate these supporting files, --completion-script-bash and --completion-script-zsh. However, my main concern is with package distribution for owners tools which use kingpin.

Consider this: when distributing a package, (specifically a .deb), the package maintainer will usually add a bash_completion script to the package to be installed in /etc/bash_completion.d. Adding these flags does help, but it will need to be made clear bash completion doesn't come for free - If they want end users to get completions, they need to install these (generated) completion files in their package distributions.

Alternatively, package maintainers need to inform users that they can add additional lines to their .bash_profile and .zshrc such as:

eval "$(my-cli-tool --completion-script-bash)"

However, personally, I feel this isn't a great end user experience. Anyway, with that all being said, it's definitely the right way to go to move the scripts into kingpin, where a package maintainer can generate these scripts after compilation and add them to the distribution.

2.

I originally felt that keeping completions outside of the app was the best idea, as it's more isolated and controlled (plus an easier feature IMO :P ), but, it didn't give users of kingpin great options for handling dynamic arguments without re-generating the script every time.

A good example of such a use case is where you have a CLI tool which reads from an hosts file and provides completion of possible known hosts. By reaching into the application and performing a HintAction we can read new options on the fly without any re-generation. I think this will be made more clear with examples.

3.

I originally attempted this, and also implemented a hybrid, however due to how bash (and ZSH) handle completion selection, a space will always be added (unless there are multiple options to choose from).

The original implementation I speak of would output every possible --<flag>=<possible option for flag> combination, but ended up being very messy in the case that the user was unsure initially of which flag to type (ie, just typing --).

docs

I've added documentation and tests. Let me know if you've got any additional comments :)

The tests are failing, but once they're fixed up I'll take another look. Thanks!

I originally felt that keeping completions outside of the app was the best idea, as it's more isolated and controlled (plus an easier feature IMO :P ), but, it didn't give users of kingpin great options for handling dynamic arguments without re-generating the script every time.

That's a good point. Looking at the example completion code, it looks like it could be super powerful. Very nice.

I originally attempted this, and also implemented a hybrid, however due to how bash (and ZSH) handle completion selection, a space will always be added (unless there are multiple options to choose from).

That's a bummer, but good to know you had the same thought :)

I think once you've addressed the few minor comments I made, this LGTM.

I've made the requested modifications. I removed that args argument for HintAction since it was not used (in fact it was just left over from early development on this feature).

However, I still can't get a green build. I've attempted using a relative package import, and not importing at all. What do you suggest I do to remediate this?

The tests are failing because you're importing gopkg.in/alecthomas/kingpin.v2 in the example main.go. You need to import github.com/alecthomas/kingpin.

Apart from that, looks great, thanks!

This is brilliant, thanks so much @nickw444. Much appreciated.

Very nice, thank you @nickw444!

Hey @alecthomas just wondering when you'll be building a release with these changes for gopkg.in? Cheers

I just pushed out v2.1.11

Thanks!

@nickw444 I've noticed that the command completion in particular seems inconsistent. _examples/curl doesn't seem to work at all. Any idea why? I haven't had time to look at it.

I'll take a quick look now.

I think I may have accidentally by chance found the issue you were having:

1. Build _examples/curl.
2. Run ./curl and perform tab completion.

./curl <TAB>
file:      ftp://     gopher://  http://    https://

The shell will respond with completion that doesn't seem consistent with kingpin completion. This is because we didn't source the kingpin completion script.

Run eval "$(./curl --completion-script-zsh)". Try again:

./curl --<TAB>
--headers  --help     --timeout  --version

./curl <TAB>
(no output)

I didn't see the first output, I saw no output, as you do with your last line. However, the curl example has commands and subcommands, which don't get completed. I'd expect:

./curl <TAB>
help get post

I can reproduce this issue now. I'm sure this is because of the pattern being used to create this command line tool. Notice how the commands are instantiated from the kingpin library, rather than from an app struct:

...
get         = kingpin.Command("get", "GET a resource.").Default()
getFlag     = get.Flag("test", "Test flag").Bool()
getURL      = get.Command("url", "Retrieve a URL.").Default()
...

All the flags work because they're defined on the command.

Look at the completion example and how the commands are defined:

func addSubCommand(app *kingpin.Application, name string, description string) {
    c := app.Command(name, description).Action(func(c *kingpin.ParseContext) error {
        fmt.Printf("Would have run command %s.\n", name)
        return nil
    })
    c.Flag("nop-flag", "Example of a flag with no options").Bool()
}
...
func main() {
    ...
    addSubCommand(app, "nmap", "Additional top level command to show command completion")
    ...
}
...

They're functionally equivalent though, so I'm not sure why that would make a difference?

You're right about that. I'll take a look on Monday. Good find!

👍